Skip to content

Return ASCOM INVALID_VALUE for out-of-range integer parameters#14

Open
ivonnyssen wants to merge 7 commits into
RReverser:mainfrom
ivonnyssen:pr/integer-parameter-handling
Open

Return ASCOM INVALID_VALUE for out-of-range integer parameters#14
ivonnyssen wants to merge 7 commits into
RReverser:mainfrom
ivonnyssen:pr/integer-parameter-handling

Conversation

@ivonnyssen
Copy link
Copy Markdown
Contributor

@ivonnyssen ivonnyssen commented Apr 7, 2026

Update (force-pushed)

This branch was rebased onto current main to pick up the unnecessary_cast clippy fix already on main (commit c9ef4d5), and one additional commit was added on top:

Map serde_repr variant rejections to ASCOM InvalidValue — fixes a remaining gap where Deserialize_repr enums (e.g. DriveRate) receiving an integer that parses but matches no variant (e.g. ConformU sending TrackingRate=5 or -1) still returned HTTP 400 instead of ASCOM InvalidValue. serde_repr rejects via Error::custom directly, so the deserializer's serde::de::Error for AlpacaParseError impl now routes custom to a new InvalidValue(String) variant, and response.rs maps it to ASCOMError::invalid_value. Three regression tests added.


Summary

Replace serde_plain::from_str with a custom deserializer so Alpaca integer parameters that parse successfully but don't fit the target type return ASCOM INVALID_VALUE (error 1025) instead of HTTP 400 BadRequest.

Closes #5

Motivation

The ASCOM Alpaca spec and ConformU conformance tests require INVALID_VALUE for parseable-but-out-of-range values — e.g. sending Id=999 to a Switch with only 4 ports should return HTTP 200 with ErrorNumber: 1025, not HTTP 400.

Previously, OpaqueParams::maybe_extract used serde_plain::from_str::<T>, which surfaces a single ParseIntError for every failure mode: malformed input, negative values for unsigned targets, and values too large for the target type. Everything collapsed into Error::BadParameter → HTTP 400, so ConformU's out-of-range checks were reported as protocol errors instead of device-level errors.

How it works

Integer parsing now goes through a two-step split:

  1. str.parse::<i64>() — fails only for genuinely malformed input (AlpacaParseError::BadFormat → HTTP 400)
  2. T::try_from(i64_value) — fails only for values that parse as integers but don't fit the target (AlpacaParseError::OutOfRange → ASCOM INVALID_VALUE)
  3. Error::custom from external code (e.g. serde_repr rejecting an unknown enum variant) — surfaced as AlpacaParseError::InvalidValue → ASCOM INVALID_VALUE

i64 is the narrowest intermediate that covers the full range of every Alpaca numeric target (i8/i16/i32/u8/u16/u32/usize — including u32 transaction IDs up to 4.3B and negative i32 device values), so a single generic parse_integer<T: TryFrom<i64>> handles all of them without per-type dispatch.

The inner AlpacaParseError is carried on Error::BadParameter, and the ResponseWithTransaction<Result<T>> IntoResponse impl destructures the OutOfRange and InvalidValue variants to produce the ASCOM-formatted error response (HTTP 200 + 1025). BadFormat continues to return HTTP 400.

Changes

  • src/server/params.rs:
    • New AlpacaParseError enum (BadFormat/OutOfRange/InvalidValue) and AlpacaDeserializer implementing serde::Deserializer
    • Integer methods parse via i64 with TryFrom narrowing; float/bool/char/enum/etc. paths preserved
    • serde::de::Error::custom impl routes to InvalidValue so serde_repr variant rejections (and similar external-code rejections of syntactically valid values) get ASCOM INVALID_VALUE treatment
    • Unit tests for range vs format classification, u32-above-i32::MAX, negative-as-OutOfRange, whitespace trimming, and Deserialize_repr enum variant rejection
  • src/server/error.rs: Error::BadParameter now carries AlpacaParseError (was serde_plain::Error)
  • src/server/response.rs: BadParameter { err: OutOfRange { .. } } and BadParameter { err: InvalidValue(_) } both map to ASCOMError::invalid_value
  • Cargo.toml: drop serde_plain dependency (no longer needed)

Test plan

  • cargo check --features server,all-devices passes
  • cargo clippy --features server,all-devices clean
  • CI clippy matrix (all 7 feature combos under RUSTFLAGS=-D warnings) clean from a clean target dir
  • New unit tests in params.rs pass (13/13)
  • Manual verification with ConformU: out-of-range Switch.Id returns ASCOM 1025, not HTTP 400
  • Manual verification: malformed input (e.g. Id=abc) still returns HTTP 400
  • Manual verification with ConformU: TrackingRate Write test no longer flags out-of-enum-range values (5, -1)

@ivonnyssen ivonnyssen marked this pull request as ready for review April 12, 2026 20:36
Copilot AI review requested due to automatic review settings April 12, 2026 20:36
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Alpaca server parameter parsing to distinguish unparseable integer inputs (HTTP 400) from parseable but out-of-range integer inputs (ASCOM INVALID_VALUE / 1025), and ensures u32 request parameters can exceed i32::MAX by widening to an intermediate signed type first.

Changes:

  • Introduces a custom serde Deserializer for request parameters that parses integers via an intermediate type, enabling explicit range-error classification.
  • Adds a new Error::ParameterOutOfRange and maps it to ASCOM INVALID_VALUE in transaction-wrapped responses.
  • Adds unit tests for integer parsing/range behavior in params.rs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/server/params.rs Replaces serde_plain::from_str parsing with a custom deserializer and adds tests for integer parsing and range classification.
src/server/error.rs Adds a new ParameterOutOfRange error variant and updates HTTP status mapping in IntoResponse.
src/server/response.rs Maps ParameterOutOfRange to an ASCOM INVALID_VALUE response (HTTP 200) when returning ResponseWithTransaction<Result<T>>.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/server/params.rs
Comment thread src/server/params.rs
Comment thread src/server/response.rs
Comment thread src/server/error.rs
@ivonnyssen
Copy link
Copy Markdown
Contributor Author

ivonnyssen commented Apr 13, 2026

Analysis of Copilot review comments 3 & 4 (out-of-range parameters escaping the transaction wrapper)

Updated 2026-04-21: code snippet refreshed to match the new Error::BadParameter { err: AlpacaParseError::OutOfRange { .. } } shape after folding ParameterOutOfRange into BadParameter. The underlying gap is unchanged.

Copilot flagged that out-of-range integer errors during ClientID/ClientTransactionID extraction bypass the ResponseWithTransaction wrapper and produce HTTP 400 instead of ASCOM INVALID_VALUE. This is a valid observation — here's the full analysis and a proposed fix.

How the issue arises

ClientID and ClientTransactionID are extracted as u32 in RequestTransaction::extract() (transaction.rs:39-46), before the ResponseWithTransaction wrapper is constructed (mod.rs:106-108). The ? on line 106 propagates any error out of exec(), where it hits Error::into_response → HTTP 400.

So sending e.g. ClientID=5000000000 (valid i64, out of range for u32) returns HTTP 400 instead of HTTP 200 + ASCOM error 1025. Note that this same behavior exists on main today (serde_plain::from_str::<u32> fails → BadParameter → HTTP 400), so this is a pre-existing gap rather than something introduced by this PR.

The device parameters that ConformU actually tests (like Switch.Id or negative index values) are extracted inside the make_response closure (mod.rs:123), which is correctly wrapped in ResponseWithTransaction.

Proposed fix

Catch the out-of-range case in exec() before it escapes. The change is ~10 lines in mod.rs:

// mod.rs — add to imports:
use super::params::AlpacaParseError;
use crate::ASCOMError;

// mod.rs — replace line 106:
//     let request_transaction = RequestTransaction::extract(&mut self.params)?;
// with:
let request_transaction = match RequestTransaction::extract(&mut self.params) {
    Ok(t) => t,
    Err(Error::BadParameter {
        name,
        err: AlpacaParseError::OutOfRange { value, target_type },
    }) => {
        return Ok(ResponseWithTransaction {
            transaction: ResponseTransaction::new(None),
            response: Err::<(), _>(ASCOMError::invalid_value(format!(
                "Parameter {name:?} value {value} is out of range for {target_type}"
            ))),
        }
        .into_response());
    }
    Err(e) => return Err(e.into()),
};

This works because ResponseWithTransaction<ASCOMResult<()>> already has an IntoResponse impl, producing HTTP 200 with {"ServerTransactionID": N, "ErrorNumber": 1025, "ErrorMessage": "..."}. ClientTransactionID is correctly omitted (None) since we couldn't parse it. BadFormat parse errors, MissingParameter, and other Error variants still propagate as HTTP 400 via the final Err(e) arm.

I also considered a two-phase extraction approach (making extract() infallible and deferring errors) which would preserve a valid ClientTransactionID even when ClientID is the one that's out of range, but that adds complexity for an unlikely edge case.

Let me know if you'd like this fix included in the PR or if you'd prefer to leave it for a follow-up.

— Igor (via Claude Code)

Copy link
Copy Markdown
Owner

@RReverser RReverser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint32 overflow: ClientID and ClientTransactionID are uint32 per spec, so values above i32::MAX (e.g. 2147483648) must parse successfully. The previous code used serde_plain::from_str which parses directly to the target type with no intermediate widening, so valid uint32 values were rejected. Using i64 as the intermediate type covers the full uint32 range.

I've been re-reading this several times and I don't get it. How is parsing directly to the target type (u32) a bad thing, but using an unrelated i64 type a good thing?

Comment thread src/server/params.rs
Comment thread src/server/params.rs Outdated
Comment thread src/server/error.rs Outdated
@ivonnyssen ivonnyssen changed the title Custom integer deserializer: INVALID_VALUE for out-of-range, i64 for uint32 support Return ASCOM INVALID_VALUE for out-of-range integer parameters Apr 21, 2026
@ivonnyssen
Copy link
Copy Markdown
Contributor Author

@RReverser you're right — the PR description framing was wrong and I've rewritten it. serde_plain::from_str::<u32>("3000000000") parses fine; u32 targets were never broken. What the old code couldn't do was classify the failure mode: any integer parse error (malformed input, negative for unsigned, too large for target) collapsed into a single BadParameter → HTTP 400. ConformU and the ASCOM spec require INVALID_VALUE (HTTP 200, error 1025) for parseable-but-out-of-range values — e.g. Switch.Id=999 when the device only has 4 ports.

The i64 intermediate isn't about widening u32; it's the narrowest type that covers the full range of every Alpaca numeric target (i8/i16/i32/u8/u16/u32/usize). That lets a single generic parse_integer<T: TryFrom<i64>> split the two failure modes cleanly: str.parse::<i64>() fails only for genuinely malformed input (→ BadFormat → HTTP 400), while T::try_from(i64) fails only for values that parse as integers but don't fit the target (→ OutOfRange → ASCOM 1025).

I've also addressed your three inline comments in the latest push:

  • ParameterOutOfRange folded into BadParameter — the variant is gone; Error::BadParameter now carries an AlpacaParseError (BadFormat or OutOfRange), and ResponseWithTransaction<Result<T>>::into_response destructures the inner variant to route HTTP 400 vs ASCOM 1025.
  • serde_plain dropped — removed from Cargo.toml (dep + server feature) and all source references.
  • ALPACAAlpaca — fixed throughout params.rs.

ivonnyssen added a commit to ivonnyssen/ascom-alpaca-rs that referenced this pull request Apr 22, 2026
ivonnyssen and others added 7 commits May 11, 2026 18:19
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Alpaca is a proper noun, not an acronym.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Make AlpacaParseError pub(crate) and carry it directly on Error::BadParameter.
The ResponseWithTransaction<Result<T>> IntoResponse impl now destructures the
inner AlpacaParseError::OutOfRange to produce ASCOM INVALID_VALUE, while plain
BadFormat errors continue to return HTTP 400.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The custom AlpacaDeserializer now handles all Alpaca parameter parsing, so
the serde_plain helper crate is no longer needed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When a `Deserialize_repr` enum (e.g. `DriveRate`) receives an integer
that's syntactically valid but matches no variant, serde_repr's
generated code calls `Error::custom("invalid value: N, expected one
of: ...")` directly. The previous `impl serde::de::Error for
AlpacaParseError` mapped that to `BadFormat`, which surfaced as
HTTP 400 — diverging from the Alpaca spec, which requires HTTP 200
with `ErrorNumber=0x401` (`InvalidValue`). ConformU's `TrackingRate
Write` test flagged this for `TrackingRate=5` and `TrackingRate=-1`.

Our own `deserialize_*` paths never return through `custom`: format
errors go directly to `BadFormat`, and primitive-range errors go
directly to `OutOfRange`. So everything reaching `custom` is by
construction a semantic rejection of a syntactically valid value
— ASCOM `InvalidValue`.

Add an `InvalidValue(String)` variant, route `custom` to it, and add
a response.rs arm that maps it to `ASCOMError::invalid_value`,
preserving serde_repr's "expected one of: ..." message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return INVALID_VALUE instead of BadRequest for integers out of range

4 participants